Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add setting to send diagnostics back to SN #463

Merged
merged 8 commits into from
Sep 18, 2023
Merged

Add setting to send diagnostics back to SN #463

merged 8 commits into from
Sep 18, 2023

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Aug 31, 2023 •
edited
Loading

This should help to investigate #411.

This adds a new opt-in setting allow diagnostics:

2023-09-09-013253_296x208_scrot

2023-09-09-021358_510x404_scrot

I added a new endpoint /api/log where diagnostics data from devices of stackers which have this setting enabled are sent. This endpoint is unauthenticated for privacy reasons.

To know which events belong to the same client, a fancy random name is generated which is only known to the client. Hence if a stacker uses two different devices, it will show up with two different names. This should make sending these events pseudonymous since we could trace back to the individual stacker if the stacker tells us their fancy name. It would only be anonymous if this step is not reversible.

Additionally, anything sent to that endpoint is also sent into a Slack channel:

sample log in slack:
2023-09-09-013046_1058x773_scrot

Since the endpoint is unauthenticated, you don't even need an account to send something to it. This may be a problem but if we require authentication, this seems antithetical to

without a doubt doesn't violate user privacy

-- #411 (comment)

Additionally, I think using authentication doesn't even really help against potential spam since anyone can easily create an account.

@ekzyis ekzyis added the feature new product features that weren't there before label Aug 31, 2023
@ekzyis ekzyis marked this pull request as draft August 31, 2023 07:50
@ekzyis ekzyis force-pushed the 411-client-side-logging branch 2 times, most recently from 47b551f to 81dc90c Compare September 6, 2023 21:47
@ekzyis ekzyis force-pushed the 411-client-side-logging branch from 81dc90c to a8d17de Compare September 8, 2023 22:56
@ekzyis ekzyis changed the title Client-side logging Add setting to send diagnostics back to SN Sep 8, 2023
@ekzyis ekzyis force-pushed the 411-client-side-logging branch 2 times, most recently from 7a85474 to a56516f Compare September 9, 2023 00:16
@ekzyis ekzyis marked this pull request as ready for review September 9, 2023 00:19
@ekzyis
Copy link
Member Author

ekzyis commented Sep 9, 2023

Asked for feedback here: https://stacker.news/items/248388

huumn
huumn reviewed Sep 12, 2023
View reviewed changes
components/logger.js Outdated

function detectOS () {
const userAgent = window.navigator.userAgent
const platform = window.navigator?.userAgentData?.platform || window.navigator.platform
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you check for navigator but you don't the line before. I think I recall navigator not being defined in some browsers. It might be best to bail out of os detection if navigator isn't defined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied that code from SO. Not sure why there is this inconsistency.

Also, navigator should be supported by every browser. We may had this error because of SSR?

But I'll use window.navigator? anyway. Better be safe than sorry :)

@ekzyis ekzyis force-pushed the 411-client-side-logging branch 2 times, most recently from a56516f to dd2eb1c Compare September 12, 2023 13:58
@ekzyis
Copy link
Member Author

ekzyis commented Sep 12, 2023

Since we discussed hiding the setting and only enabling it for us manually ... should we just hide the setting?

The advantage would be that we don't need to explain what it does.

The disadvantage would be that it's less transparent that this exists and some stackers may actually want to help us.

I'll also try to fix the error boundary in this PR. So we'll get more useful information out of these changes already.

ekzyis
ekzyis commented Sep 12, 2023
View reviewed changes
pages/api/log/index.js Outdated Show resolved Hide resolved
@ekzyis ekzyis marked this pull request as draft September 12, 2023 14:31
@huumn
Copy link
Member

huumn commented Sep 12, 2023

Since we discussed hiding the setting and only enabling it for us manually ... should we just hide the setting?

Nah let's keep it there. I'll do a pass on the copy before/after merging.

@ekzyis
Add diagnostics settings & endpoint
5977322 
Stackers can now help us to identify and fix bugs by enabling diagnostics.

This will send anonymized data to us.

For now, this is only used to send events around push notifications.
@ekzyis
Send diagnostics to slack
29c730b
@ekzyis
Detect OS
b76cc66
@ekzyis
Diagnostics data is only pseudonymous, not anonymous
1570dee 
It's only pseudonymous since with additional knowledge (which stacker uses which fancy name), we could trace the events back to individual stackers.

Data is only anonymous if this is not possible - it must be irreversible.
@ekzyis
Check if window.navigator is defined
5b6fefa
@ekzyis
Use Slack SDK
0810dad
@ekzyis
Catch errors of slack requests
0b4e285
@ekzyis ekzyis force-pushed the 411-client-side-logging branch from dd2eb1c to 0b4e285 Compare September 13, 2023 11:58
@ekzyis
Copy link
Member Author

ekzyis commented Sep 13, 2023

Using the Slack SDK now.

The only (minor, imo) issue is that messages from a client arrive out of order in slack since we don't wait until the previous slack requests are done before submitting new slack requests.

Doing this would require a mutex in the server or client afaict. We can't just use await somewhere since the slack requests are triggered in different API requests which are triggered at different points in the client code.

But I think this fine for the first iteration.

I also looked into the error boundary. I noticed that getDerivedStateFromError is triggered but not componentDidCatch. Also, ErrorBoundary has no access to me, logger etc. so it would require a more elaborate solution to send errors from there to /api/log.

So I think it's also not worth it to dig into that further - at least for now. We can use this as-is and see if that even works well. If too many people start using it, the messages being out of order may become more important so that should be fixed in that case first before anything else.

@ekzyis ekzyis marked this pull request as ready for review September 13, 2023 13:37
@huumn
Copy link
Member

huumn commented Sep 18, 2023 •
edited
Loading

Overall it looks good. The only real concern I have with this is storing the log messages in the application db when this may be kind of noisy/a lot to store.

It might be worth removing the db storage until we have somewhere less permanent to store them. idk I need to think about this a little more.

@huumn
Copy link
Member

huumn commented Sep 18, 2023

Going to merge (and possibly comment out the db storage)

@huumn huumn merged commit 3a7c3f7 into stackernews:master Sep 18, 2023
@ekzyis ekzyis mentioned this pull request Sep 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huumn huumn huumn left review comments

Assignees
No one assigned
Labels
feature new product features that weren't there before
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ekzyis @huumn